Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miri: properly check return-position noalias #106212

Closed
wants to merge 3 commits into from

Conversation

RalfJung
Copy link
Member

As discussed in rust-lang/unsafe-code-guidelines#385, Box-returning functions get a noalias on their return value, which has a bunch of extra aliasing rules that Miri did not properly check.

We now do this by marking the post-return retags as FnReturn, and then when retagging a Box in that vein we clear the borrow stack of all the other tags. On its own this change is incompatible with the weak protectors on Box arguments that I recently introduced to fix Box noalias enforcement, so we replace those protectors by also doing such a "clear" retagging in FnEntry for Box. Conveniently this means we can remove weak protectors entirely while still properly enforcing noalias (as far as I can tell).

Basically this means that Box cannot be reborrowed; when they are passed to and from a function, they always must be the only pointer used henceforth for this allocation. With Box not being a reference type, this kind of makes sense I guess. There is a chance that this might break even more unsafe code that uses Box in creative ways, though that seems unlikely -- when a Box is passed to a function, I would expect that function to either deallocate or return the Box; the new point now is that the caller must use the returned box rather than considering this a "reborrow" and sticking to some sneaky copy of the old box. Such code would be unsound with return-position noalias anyway.

@saethlin I hope this doesn't break the tag cache in subtle ways, though given that it can handle clearing the stack for the "unknown bottom" situation, I assume that will also work here.

One odd thing that can now happen is that the "base tag" of an allocation might cease to be valid for that allocation. But I don't think we ever assumed that it would always remain valid, and this does seem to match the semantics of return-position noalias, so I think this is fine.

r? @oli-obk @saethlin

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 28, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2022

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 9b889e53e78a186a861a8407c225f9d8e0d436f5 and a81054320b615da05fe0c3119fe339b51114f1c9
Tool subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---

actual output differed from expected
--- tests/pass/box-custom-alloc.stderr
+++ <stderr output>
+error: Undefined Behavior: trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
+   |
+   |
+LL | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
+   | |
+   | |
+   | trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
+   | this error occurs as part of retag at ALLOC[0x0..0x1]
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+help: <TAG> was created here, as the base tag for ALLOC
+   |
+   |
+LL |     let mut space = vec![MaybeUninit::new(0); 1];
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: <TAG> was later invalidated at offsets [0x0..0x1] by a Unique Box retag
+   |
+   |
+LL |     let boxed = Box::new_in([42u8; 1], &once_alloc);
+   = note: BACKTRACE (of the first span):
+   = note: BACKTRACE (of the first span):
+   = note: inside `std::ptr::drop_in_place::<[std::mem::MaybeUninit<u8>]> - shim(None)` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+   = note: inside `<std::vec::Vec<std::mem::MaybeUninit<u8>> as std::ops::Drop>::drop` at RUSTLIB/alloc/src/vec/mod.rs:LL:CC
+   = note: inside `std::ptr::drop_in_place::<std::vec::Vec<std::mem::MaybeUninit<u8>>> - shim(Some(std::vec::Vec<std::mem::MaybeUninit<u8>>))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+  --> $DIR/box-custom-alloc.rs:LL:CC
+   |
+LL | }
+   | ^
---
+


There were 1 unmatched diagnostics that occurred outside the testfile and had no pattern
    Error: Undefined Behavior: trying to retag from <3064> for Unique permission at alloc1631[0x0], but that tag does not exist in the borrow stack for this location
full stderr:
full stderr:
error: Undefined Behavior: trying to retag from <3064> for Unique permission at alloc1631[0x0], but that tag does not exist in the borrow stack for this location
   |
   |
LL | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
   | |
   | |
   | trying to retag from <3064> for Unique permission at alloc1631[0x0], but that tag does not exist in the borrow stack for this location
   | this error occurs as part of retag at alloc1631[0x0..0x1]
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3064> was created here, as the base tag for alloc1631
   |
   |
LL |     let mut space = vec![MaybeUninit::new(0); 1];
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <3064> was later invalidated at offsets [0x0..0x1] by a Unique Box retag
   |
   |
LL |     let boxed = Box::new_in([42u8; 1], &once_alloc);
   = note: BACKTRACE (of the first span):
   = note: BACKTRACE (of the first span):
   = note: inside `std::ptr::drop_in_place::<[std::mem::MaybeUninit<u8>]> - shim(None)` at /checkout/library/core/src/ptr/mod.rs:490:1: 490:56
   = note: inside `<std::vec::Vec<std::mem::MaybeUninit<u8>> as std::ops::Drop>::drop` at /checkout/library/alloc/src/vec/mod.rs:3054:13: 3054:91
   = note: inside `std::ptr::drop_in_place::<std::vec::Vec<std::mem::MaybeUninit<u8>>> - shim(Some(std::vec::Vec<std::mem::MaybeUninit<u8>>))` at /checkout/library/core/src/ptr/mod.rs:490:1: 490:56
  --> tests/pass/box-custom-alloc.rs:56:1
   |
LL | }
   | ^

@JakobDegen
Copy link
Contributor

Can we just stop emitting ret-position noalias instead? I'm not fundamentally opposed to having semantics as suggested here, but I can't really claim to understand all the consequences of it and actually supporting allocators reasonably well needs a lot more work

@RalfJung
Copy link
Member Author

RalfJung commented Dec 28, 2022 via email

@RalfJung
Copy link
Member Author

Ah, but this is a problem for boxes with custom allocators.

@saethlin
Copy link
Member

I hope this doesn't break the tag cache in subtle ways
...
One odd thing that can now happen is that the "base tag" of an allocation might cease to be valid for that allocation. But I don't think we ever assumed that it would always remain valid

Yes, I think the new clear-on-return logic is correctly handled by the same code path that handles the creation of an unknown bottom. Bless whoever thought of factoring the code in this way instead of writing the base tag over the whole cache ;)

@saethlin
Copy link
Member

that RFC needs to be written by someone else

Working on it, slowly. Keep mentioning this to fuel the fire. I'm hoping I'll shop around a pre-RFC at the end of January.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2023

Can we just stop emitting ret-position noalias instead?

You got your wish. ;) This PR is superseded by #106371.

@RalfJung RalfJung closed this Jan 2, 2023
@RalfJung RalfJung deleted the ret-position-noalias branch January 2, 2023 14:14
bors added a commit to rust-lang/miri that referenced this pull request Jan 2, 2023
tweaks to retag diagnostic handling

Salvaged from rust-lang/rust#106212
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants